aggregator: unify resolver implementation and tests #46623
Conversation
k8s-github-robot
added
size/XL
k8s-ci-robot
added
the
cncf-cla: yes
sttts
force-pushed
the
sttts-aggregator-unified-modes
branch
from
5d2838e
to
4ed36e9
Compare
| } | ||
|
|
||
| return svc, "https", "443", nil |
There was a problem hiding this comment.
These seem like good defaults but if the service object already has the port and scheme, then either grab those and return them or just return the service and leave it to the caller to worry about extracting/determining defaults. (We already seem to handle the port portion of this in your findServicePort method) This also suggests we are dropping support for "HTTP" when talking to aggregated api servers. I think thats probably a good thing but I want to make sure its a deliberate choice.
| // Find a Subset that has the port. | ||
| for ssi := 0; ssi < len(eps.Subsets); ssi++ { | ||
| ss := &eps.Subsets[(ssSeed+ssi)%len(eps.Subsets)] | ||
| if len(ss.Addresses) == 0 { | ||
| continue | ||
| } | ||
| for i := range ss.Ports { | ||
| if ss.Ports[i].Name == portStr { | ||
| if ss.Ports[i].Name == svcPort.Name { |
There was a problem hiding this comment.
Not for now but there is something odd about the case where we are asked for a port number match, find the first matching port number and the pick all the routes which match that routes port name. If ports are consistently names there won't be an issue but if they aren't the behavior is going to be very hard to debug.
sttts
force-pushed
the
sttts-aggregator-unified-modes
branch
2 times, most recently
from
6911648
to
96dba6a
Compare
|
@cheftako addressed your comments. Also removed the remaining forbidden import in the tests. Ready for final review. |
sttts
force-pushed
the
sttts-aggregator-unified-modes
branch
from
96dba6a
to
869fab5
Compare
sttts
added
release-note-none
|
lgtm |
k8s-github-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| } | ||
| return nil, errors.NewServiceUnavailable(fmt.Sprintf("no service port %q found for service %q", port.String(), svc.Name)) | ||
| } | ||
|
|
||
| // ResourceLocation returns a URL to which one can send traffic for the specified service. | ||
| func ResolveEndpoint(services listersv1.ServiceLister, endpoints listersv1.EndpointsLister, namespace, id string) (*url.URL, error) { |
There was a problem hiding this comment.
You have change the meaning of the id field (it used to be scheme : service name : port) it is now just the service name. I believe the caller at either https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go#L153-L155 or https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go#L116 will need to be fixed to reflect this.
There was a problem hiding this comment.
I did. But I am struggling to find out where we decide/document that a service name (which comes from APIService as far as I see) can be in this colon syntax.
There was a problem hiding this comment.
Ran your fix on my GCE cluster. Despite my worries looks like it works just fine.
The service name has always been intended to be just a name, all the way back to the initial comment from @lavalamp about doing so here back in november: #37561 (comment) . If it changed, then it was an accidental quirk of implementation that change the API from its intended and documented shape. If we want a port, that port name or int should be structured in the API as such and not glommed as a string. And I was strongly swayed by Daniel's point in the initial version of the API. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako, deads2k, sttts
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
sttts
force-pushed
the
sttts-aggregator-unified-modes
branch
from
869fab5
to
c7d9a39
Compare
sttts
added
lgtm
|
Automatic merge from submit-queue (batch tested with PRs 43505, 45168, 46439, 46677, 46623) |
This is #45082, but without the port support.